-
Notifications
You must be signed in to change notification settings - Fork 2
fix(test): Fuzzing testing fail #1261
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Daniil Antoshin <[email protected]>
0031a5a
to
ec7a88a
Compare
Signed-off-by: Daniil Antoshin <[email protected]>
4b337eb
to
9a19147
Compare
Signed-off-by: Daniil Antoshin <[email protected]>
Signed-off-by: Daniil Antoshin <[email protected]>
Signed-off-by: Daniil Antoshin <[email protected]>
Reviewer's GuideThis PR refines the HTTP fuzzing framework by standardizing on testing.TB, improving error handling for header fuzzing, cleaning up unused code, adjusting upload locking semantics, and updating fuzz scripts and Dockerfiles with dependency and caching enhancements. Class diagram for updated HTTP fuzzing frameworkclassDiagram
class fuzzRequest {
+Fuzz(tb testing.TB, data []byte, method string, addr string) *http.Request
}
class ProcessRequests {
+ProcessRequests(tb *testing.T, data []byte, addr string, methods ...string)
}
class ProcessRequest {
+ProcessRequest(tb testing.TB, data []byte, addr string, method string)
}
class fuzzHTTPRequest {
+fuzzHTTPRequest(tb testing.TB, fuzzReq *http.Request) *http.Response
}
ProcessRequests --> ProcessRequest
ProcessRequest --> fuzzRequest : uses
ProcessRequest --> fuzzHTTPRequest : uses
fuzzRequest --> http.Request : returns
fuzzHTTPRequest --> http.Response : returns
Class diagram for uploader locking changeclassDiagram
class uploadServerApp {
+processUpload(irc imageReadCloser, w http.ResponseWriter, r *http.Request)
+upload(readCloser, cdiContentType, dvContentType, headers)
-mutex
}
uploadServerApp : processUpload() now locks before calling upload()
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- The loop over header values was changed to “for range len(v)”, which won’t compile—iterate directly over the slice/string (e.g. “for range v”) or use a proper index loop.
- In processUpload you’re holding the mutex for the duration of the upload call; move the app.upload invocation outside the lock so only the state mutation is protected.
- You removed the fuzzworker cleanup in fuzz.sh—consider reintroducing a cleanup step or refactoring it to avoid orphaned processes after fuzzing.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The loop over header values was changed to “for range len(v)”, which won’t compile—iterate directly over the slice/string (e.g. “for range v”) or use a proper index loop.
- In processUpload you’re holding the mutex for the duration of the upload call; move the app.upload invocation outside the lock so only the state mutation is protected.
- You removed the fuzzworker cleanup in fuzz.sh—consider reintroducing a cleanup step or refactoring it to avoid orphaned processes after fuzzing.
## Individual Comments
### Comment 1
<location> `images/dvcr-artifact/fuzz.Dockerfile:6` </location>
<code_context>
build-essential \
- libnbd-dev
+ libnbd-dev \
+ qemu-utils
WORKDIR /app
</code_context>
<issue_to_address>
**🚨 question (security):** Adding qemu-utils increases image size and attack surface.
If qemu-utils is not essential for fuzzing, please reconsider its inclusion due to the increased image size and attack surface.
</issue_to_address>
### Comment 2
<location> `images/dvcr-artifact/pkg/uploader/uploader.go:348` </location>
<code_context>
w.WriteHeader(http.StatusBadRequest)
}
- err = app.upload(readCloser, cdiContentType, dvContentType, parseHTTPHeader(r))
-
app.mutex.Lock()
defer app.mutex.Unlock()
+ err = app.upload(readCloser, cdiContentType, dvContentType, parseHTTPHeader(r))
+
if err != nil {
</code_context>
<issue_to_address>
**issue (performance):** Moving upload call inside mutex may affect concurrency.
This change may serialize uploads, impacting performance in high-throughput use cases. Confirm if this is necessary for thread safety.
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Description
This PR fix bring some fixes for fuzzing tests.
Checklist
Changelog entries